-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide a bundled flag for gdal-sys
#517
base: master
Are you sure you want to change the base?
Conversation
Cargo.toml
Outdated
|
||
|
||
[patch.crates-io] | ||
proj-sys = { git = "https://github.com/GiGainfosystems/proj", rev = "54716dd8955d4f0561ce9bf8a83610b605e3c007" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently relays on a non-accepted patch to proj-sys
: georust/proj#190
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed 27c2a5e which changes this to use the master branch of proj-sys instead as the relevant patch is now merged there. There is still no release that contains this change yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we still need georust/proj#192
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's finally released with proj-sys 0.24 🎉
gdal-src/build.rs
Outdated
.define("BUILD_GMOCK", "OFF") | ||
.define("PROJ_INCLUDE_DIR", format!("{proj_root}/include")) | ||
.define("PROJ_LIBRARY", format!("{proj_root}/lib/{proj_lib}")) | ||
// enable the gpkg driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I require the geopackage driver for my usecase that's why I added these configurations. In the end we already depend on libsqlite3
via proj, so that should hopefully be fine.
That written: Anyone that tries to actually use that driver to create a geopackage dataset will likely run into: OSGeo/gdal#9135 (which hopefully will be fixed soon upstream, so it might be worth to pull in the fix into the bundled version afterwards.)
Marked as draft as I need to perform a final set of tests on MacOS and Windows. |
@@ -1,6 +1,7 @@ | |||
# Changes | |||
|
|||
## Unreleased | |||
- Add a `bundled` feature for `gdal-sys` that allows to build and statically link a minimal bundled version of gdal during `cargo build` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably mention that it only supports SQLite, GPKG, GeoTIFF, some other less popular formats, and probably no compression. I'm afraid that users will see this and expect it to work for COG, JPEG2000, NetCDF and so on.
Also see https://www.mail-archive.com/[email protected]/msg40172.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following 19 drivers are supported by this configuration according to DriverManager::all()
:
Virtual Raster
Derived datasets using VRT pixel functions
GeoTIFF
Cloud optimized GeoTIFF generator
Erdas Imagine Images (.img)
In Memory Raster
Geographic Network generic file based model
Geographic Network generic DB based model
ESRI Shapefile
MapInfo File
VRT - Virtual Datasource
Memory
Keyhole Markup Language (KML)
GeoJSON
GeoJSON Sequence
ESRIJSON
TopoJSON
GeoPackage
SQLite / Spatialite
gdal-src/build.rs
Outdated
"libproj.a" | ||
}; | ||
|
||
let res = cmake::Config::new("source") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone wondering, cmake
automatically sets CMAKE_BUILD_TYPE
.
This looks all right, but as I mentioned in the related issue, I'm worried it's only going to be useful to you. GDAL is pretty large, and its users might have a lot of expectations, but a version that only supports GPKG is going to be very surprising. Even your use case might be better served by https://docs.rs/geozero. CC @rouault long-term, how do you feel about ExternalProject (SuperBuild)? |
a30248e
to
1803cfd
Compare
I agree with the point that users might have expectations that are not covered (yet) by the proposed bundling support, but I disagree with with the point that it only supports As for other drivers: It is certainly possible to enable other drivers as well. That mostly requires going through the list of supported drivers and classify them if they need external dependencies or not. The later ones can easily be enabled, while for the former ones more work is required. Depending on the required external dependency that might require adding another sys crate as dependency and enabling the bundling support there (like for example for
Trust me if I say that no geozero won't solve my use-case as it has to much assumptions around which geometry types exist and how they should be handled for my use-case. (It's specifically not about translating from one geometry format to another). |
Yes, I think it's all right. We should probably document our features anyway, and that's a good place where we can explain the limitations of the bundled GDAL. |
I just checked the documentation. GDAL already provides a list of dependencies for each driver: https://gdal.org/drivers/vector/index.html Anything that lists "Built-in by default" as dependency can probably just be enabled without problems. I also know that bundling support exists for the following other dependencies:
So yes, it's probably possible to just enable most of the expected drives, although I personally would prefer having all the native dependencies behind separate feature flags. |
1803cfd
to
1315e16
Compare
I miss some contextual elements to understand your question |
If you're not familiar with it, ExternalProject is a CMake component that can download libraries, apply patches, then link your project with them. So it can be useful for people who want a custom build but don't want to build every dependency manually. If GDAL adds that in the future, we can integrate with it without creating Rust libraries for that bundle the source code of the GDAL dependencies. If it doesn't, that's fine, people can still use GeoTIFF, GPKG, SHP and a a few other formats with the approach here. |
ok, I see. That's something I've considered, but I'd be hesitant to go into that business, as it equates to creating yet another packaging system, and for GDAL, with all its transitive dependencies, that can mean ~ 100 libraries for a full build... That said I do recognize that might be a recurring need for people in "unstandard" environments (Android, WASM, etc.) to have to rebuild the whole world. I'm not sure if that would belong to the OSGeo/GDAL repo itself, or if it might be a side repository where people from different teams can collaborate, and be independent of GDAL development itself. Might be worth raising the idea on gdal-dev. |
Yeah, that makes sense. You've already got a full plate with GDAL and the other libraries you're maintaining, you don't need to get into a whole new packaging system unless you really want to. It would only make a difference for this PR if you were already planning to add it. |
1315e16
to
8db29ac
Compare
8db29ac
to
e1d7faf
Compare
cmake = "0.1.50" | ||
|
||
[features] | ||
default = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to discussion which drivers should be build by default as cargo feature flags. We cannot easily say build everything and allow users to opt out as cargo feature flags are additive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess NetCDF and Zarr could be interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The zarr driver so currently not not supported by the bundling. It would require figuring out how to bundle all the dependencies for that driver as well.
Netcdf is supported, as there is already a rust crate which provides bundling support and exposes the relevant information. Therefore it should be possible to enable it by default.
I've pushed a new version of the build script that adds feature flags for a lot of the built in drivers. If I enable all of them List with 162 driver names
Notably this is still missing support for anything that depends on It's up to discussion which of these drivers should be enabled by default. |
e1d7faf
to
54ef44f
Compare
I confirmed that this works on linux (x86_86), windows (msvc, x86_64) and macos (aarch64). |
e653b39
to
f1793e3
Compare
…s before any depentend crate
I'm away from the computer until Monday, but I've been meaning to get back to this afterwards, especially at the latest changes. But I think we can merge it soon. We can also fix things in later PRs if anything comes up. |
No worries, take your time off. It's not that important to merge it literally in this moment. The comment was mostly meant to bring this PR back to your attention. |
I'm still a bit worried about the massive complexity here, but there's not much we can do about it. So LGTM and thanks for the massive effort you've put into this! Looks like this still needs a rebase, and there's a pretty critical fix in #550 which I hope we can release once someone reviews it. Then I think we should be good to go, unless anyone speaks out. |
If there is anything I can do to lower the complexity, just let me know. That written: Most of the changes are just in the new
I'm not sure about that. Github doesn't show any conflicts so it should be possible to merge it directly? Or is there any policy to have everything always on top of the current master state? There is one other thing that you might want to consider doing before merging: This currently relies on a unpublished proj4-sys version. It might be meaningful to cut a release there first and then change the dependencies here to the released version? |
I don't think so, something has to drive the 10KLOC of CMake that GDAL has 🙂.
You're right, I assumed it wasn't going to work because I had Rebase selected in the GitHub UI:
I don't expect us to publish 0.18 too soon, we're doing one release per year or so. But yeah, ideally we'd use a published version there. |
@lnicola This is another bump. Would it be possible to make finally some progress here? |
I wonder what we can do to get that |
@lnicola I've updated the PR to use the newly published proj-sys version |
Yup, that's great. I guess this is now blocked on:
|
@ChristianBeilschmidt @metasim Are there any chances to get another review on this? |
Simplify hdf5/netcdf bundling
I will try to test it this weekend. |
The current CI failures are very likely caused by the rust 1.81 release, which brings new clippy lints. I might have a look at them tomorrow. |
#552 fixes the lint. |
So i am really worried about maintaining this. Is it really a good idea to have a gdal submodule? Can't we curl the release.tar.gz we want to use and build that? |
I would rather not repeat the existing discussion on this topic, I think all options were already discussed there. The short version is:
EDIT: I also do not really understand why this would make it harder to maintain this. You only need to interact with the subrepo if you want to update the source code. In that case you would also need to update the cd gdal-src/source
git fetch
git checkout v$WHATEVER_IS_THE_NEW_GDAL_VERSION
cd ..
git add source
git commit -m "Update gdal source" (These steps are also documented in the At least to me that seems to be easier than searching the download link for the |
This commit introduces a new
gdal-src
crate which bundles gdal and builds a minimal version from source viabuild.rs
Fixes #465
CHANGES.md
if knowledge of this change could be valuable to users.